Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[GH-178] Assign moderator to only the user who start the meeting #211

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[GH-178] Assign moderator to only the user who start the meeting #211

wants to merge 2 commits into from

Conversation

ngoan98tv
Copy link
Contributor

@ngoan98tv ngoan98tv commented Apr 8, 2022 •
edited
Loading

Summary

Adding field moderator to JWT claims so it can work with jitsi-token-moderation-plugin to identify moderator.

Now, only the user who start the meeting can be moderator

Ticket Link

Fixes #178

@ngoan98tv ngoan98tv requested a review from larkox as a code owner April 8, 2022 07:52
@larkox larkox requested a review from mickmister April 8, 2022 07:59
larkox
larkox approved these changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The only question I have is if it is somehow possible to "trick the system" by somehow passing someone else's user id.

@larkox larkox added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Apr 8, 2022
@ngoan98tv
Copy link
Contributor Author

hmm, there is one chance that someone replace creator_id in jwt payload, but then the token will be invalid when verifying.

I see no way to fake the moderator for now

@ngoan98tv ngoan98tv changed the title Fix #178: assign moderator to only the user who start the meeting [GH-178] Assign moderator to only the user who start the meeting Apr 8, 2022
mickmister
mickmister approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

webapp/src/components/root_portal.tsx
Comment on lines +15 to +21
export class InjectionProvider extends React.Component<any> {
public render(): JSX.Element {
const stores = {...this.props};
delete stores.children;
return React.createElement(Provider as any, stores, this.props.children);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fixes this one

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there should be a cleaner solution for a lint issue like this 🤷 Any thoughts on how this change may introduce a bug? Was this solution sourced from somewhere on the web?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the solution here mobxjs/mobx-react#342 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jespino Curious what you think about this. Probably insignificant

@mickmister mickmister removed the 2: Dev Review Requires review by a core committer label Apr 25, 2022
@dipak-demansol dipak-demansol requested review from DHaussermann and dipak-demansol and removed request for dipak-demansol May 12, 2022 09:23
@agarciamontoro agarciamontoro mentioned this pull request Jun 9, 2022
Merged
@agarciamontoro
Copy link
Contributor

I added the changes in root_portal.tsx in #214, which I just merged, and I expected this PR to have merge conflicts, but it doesn't 🤔 Anyway, heads-up that those changes are already in master.

@hanzei hanzei removed the request for review from dipak-demansol December 2, 2022 15:23
@hanzei hanzei requested review from AayushChaudhary0001 and removed request for DHaussermann October 16, 2023 20:19
@DaDummy
Copy link

DaDummy commented Jan 13, 2024 •
edited
Loading

Out of curiosity as a present user of this mattermost plugin: Is this change configurable/optional and disabled by default or will this be a breaking change?

I'm asking because we actually prefer the current behavior of gaining moderation privileges by using the link in mattermost and then generating a non-privileged link in the meeting to use for external invitations. So this change would break our workflow if not disabled on our instance.

@mickmister
Copy link
Contributor

@DaDummy Thanks for commenting here. Sure, I think it makes sense to have this be configurable

@ngoan98tv Are you interested in implementing this? Also, would be good to know if you would be open to address any concerns that come up in general with this, as this was submitted quite a while ago. Apologies for the large delay in review here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mickmister mickmister mickmister approved these changes

@larkox larkox larkox approved these changes

@AayushChaudhary0001 AayushChaudhary0001 Awaiting requested review from AayushChaudhary0001

Assignees
No one assigned
Labels
3: QA Review Requires review by a QA tester
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for jitsi-token-moderation-plugin?
5 participants
@ngoan98tv @agarciamontoro @DaDummy @mickmister @larkox